Skip to content

Add OPENSIM_DISABLE_STATIC_TYPE_REGISTRATION cmake option#4012

Merged
adamkewley merged 1 commit intomainfrom
feature_opensim-disable-static-type-registration
Feb 24, 2025
Merged

Add OPENSIM_DISABLE_STATIC_TYPE_REGISTRATION cmake option#4012
adamkewley merged 1 commit intomainfrom
feature_opensim-disable-static-type-registration

Conversation

@adamkewley
Copy link
Contributor

@adamkewley adamkewley commented Feb 6, 2025 •
edited
Loading

This is a patch I've been applying downstream in opensim-creator that I figured might be useful for opensim-core.

This adds an OPENSIM_DISABLE_STATIC_TYPE_REGISTRATION cmake option that can be toggled ON to build OpenSim without the static-init-time instantiator classes.

The motivation for doing this is:

  • It lets downstream applications/libraries decide if/when to register OpenSim types. There are several reasons why downstream might want this:
    • The application wants to set up logging, log sinks, etc. before registration, so that all logged registration errors appear in a runtime-defined location (e.g. a UI).
    • The application wants to lazily-load parts of OpenSim
    • A bug has appeared in the application and it would be useful to debug step registration, skip registering something, etc. without having to recompile OpenSim.
  • It lets downstream application/libraries decide load-order. The reason you'd want to do this is:
    • OpenSim is being loaded as one library/application, so the instantiators cannot lean on the fact that the dynamic loader is loading libraries in use-order. Right now, the only reason the static initializers work is because osimActuators.dll depends on osimCommon.dll, so the dynamic loader just happens to load one before the other - this may fail if they are merged into one binary. It may also fail if (e.g.) the log is statically initialized after a static registration function that writes to the log (a segfault I encountered with OpenSim Creator).

Brief summary of changes

  • Added OPENSIM_DISABLE_STATIC_TYPE_REGISTRATION to CMakeLists.txt
  • Used the resulting compile define to control whether the static initializer is emitted or not

Testing I've completed

  • Similar pattern works on OpenSim Creator, which has its own application log (shown to the user via a UI) and statically compiles OpenSim into its executable.

Looking for feedback on...

CHANGELOG.md (choose one)

  • updated TODO

This change is Reviewable

nickbianco
nickbianco approved these changes Feb 11, 2025
View reviewed changes
Copy link
Member

@nickbianco nickbianco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but requesting a review from @aymanhab since (I think) he wrote the instantiators originally and may have some comments.

@nickbianco nickbianco requested a review from aymanhab February 11, 2025 17:28
aymanhab
aymanhab approved these changes Feb 11, 2025
View reviewed changes
Copy link
Member

@aymanhab aymanhab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @adamkewley that looks reasonable, especially that the default behavior is unchanged.
:lgtm:

Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @adamkewley)

CMakeLists.txt line 193 at r1 (raw file):

on using by calling `RegisterTypes_osimLIBRARY` (e.g. `RegisterTypes_osimActuators`),
or by manually registering each type (e.g. `Object::registerType(PhysicalOffsetFrame());`)." OFF)
mark_as_advanced(OPENSIM_DISABLE_STATIC_TYPE_REGISTRATION)

Will this show in cmake popup? Great explanation but maybe too verbose for a popup? Maybe something like turn it on only if you'll load the libraries explicitly in your code? but your call.

@adamkewley
Copy link
Contributor Author

adamkewley commented Feb 12, 2025

Will this show in cmake popup? Great explanation but maybe too verbose for a popup? Maybe something like turn it on only if you'll load the libraries explicitly in your code? but your call.

I haven't used the native CMakeGUI for a while (Visual Studio packages cmake these days) but, from what I understand, it'll pop up in the UI if/when the user mouses over the cache variable - assuming the user enables viewing advanced cache variables.

Although it's verbose, I figured it's probably the best place to document it because it has the dual benefit of documenting the compile define for people who grep the OPENSIM_DISABLE_STATIC_TYPE_REGISTRATION after seeing it in the C++ code, and documenting the builder-facing cache variable for people who are building OpenSim (but aren't reading the C++).

I can try and make it shorter.

@adamkewley adamkewley force-pushed the feature_opensim-disable-static-type-registration branch from 620c6a5 to d60aea1 Compare February 24, 2025 13:25
@adamkewley
Copy link
Contributor Author

adamkewley commented Feb 24, 2025

Merging: I force-pushed a shorter docstring but left the rest intact so that there's one commit for the whole change (which is easier to reverse if there's problems post-merge)

thanks @aymanhab @nickbianco

@adamkewley adamkewley merged commit f96fb71 into main Feb 24, 2025
9 of 11 checks passed
@adamkewley adamkewley deleted the feature_opensim-disable-static-type-registration branch February 24, 2025 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aymanhab aymanhab aymanhab approved these changes

@nickbianco nickbianco nickbianco approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@adamkewley @aymanhab @nickbianco